Skip to content

Add GC support for external memory handles - #133986

Open
jkoritzinsky wants to merge 26 commits into
dotnet:mainfrom
jkoritzinsky:external-roots
Open

jkoritzinsky wants to merge 26 commits into
dotnet:mainfrom
jkoritzinsky:external-roots

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

Summary

  • add AppDomain-managed external memory handles for GC-scanning managed references stored outside the GC heap and managed stacks
  • enumerate external memory handles through DAC and a dedicated cDAC ExternalMemoryHandles contract, including byref-like field walking and interior-pointer resolution
  • protect unboxed byref-like func-eval results for the lifetime of the returned ICorDebugValue

Testing

  • build.cmd tools+tools.cdactests -test
  • build.cmd clr

Note

This pull request description was generated with GitHub Copilot.

jkoritzinsky and others added 5 commits September 14, 2026 14:44
Transfer external memory handle ownership to the returned debugger value so byref-like result storage remains GC-protected for its lifetime.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 15, 2026 19:03
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

@jkoritzinsky
jkoritzinsky requested review from a team, davidwrighton and jkotas September 15, 2026 19:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect func-eval handling, disposal failure paths, and cDAC root classification.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds AppDomain-managed external memory handles so GC, DAC/cDAC, and debugger func-eval can track managed references outside the GC heap.

Changes:

  • Adds runtime handle storage, GC scanning, DAC/cDAC enumeration, and pointer resolution.
  • Extends cDAC contracts, RefWalk support, and inline-array metadata.
  • Protects byref-like func-eval results and adds tests/documentation.
File Summary
src/​native/​managed/​cdac/​tests/​UnitTests/​RefWalkExternalMemoryHandleTests.cs Tests external-root discovery and byref-like walking.
src/​native/​managed/​cdac/​tests/​UnitTests/​RefWalkExternalMemoryHandlesContractTests.cs Tests RefWalk contract integration.
src/​native/​managed/​cdac/​tests/​UnitTests/​MethodTableTests.cs Tests inline-array metadata.
src/​native/​managed/​cdac/​tests/​UnitTests/​ExternalMemoryHandlesTests.cs Tests external-handle enumeration.
src/​native/​managed/​cdac/​tests/​UnitTests/​ContractDescriptor/​TargetTests.cs Tests contract descriptor coverage.
src/​native/​managed/​cdac/​tests/​TestInfrastructure/​TestPlaceholderTarget.cs Provides test placeholder target infrastructure.
src/​native/​managed/​cdac/​tests/​DumpTests/​DacDbi/​DacDbiRefWalkDumpTests.cs Tests dump-based RefWalk behavior.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Legacy/​Dbi/​Helpers/​RefWalk.cs Integrates external roots with legacy RefWalk.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​DataType.cs Registers the external-handle data type.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Data/​ExternalMemoryHandle.cs Defines external-handle data.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Data/​EEClass.cs Reads inline-array metadata.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Data/​AppDomain.cs Describes AppDomain handle data.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​CoreCLRContracts.cs Registers and validates contracts.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Contracts/​StackWalk/​GC/​GcScanContext.cs Uses shared GC pointer-resolution support.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Contracts/​StackWalk/​GC/​GCInteriorPointerResolver.cs Resolves interior pointers.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Contracts/​RuntimeTypeSystem_1.cs Implements inline-array inspection.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Contracts/​Contracts/​ExternalMemoryHandles_1.cs Enumerates external roots. Moderate: checks GC flags for interior roots incorrectly.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Abstractions/​Contracts/​IRuntimeTypeSystem.cs Adds inline-array type inspection.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Abstractions/​Contracts/​IExternalMemoryHandles.cs Defines the external-handle API. Nit: lacks a linked API approval issue.
src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Abstractions/​ContractRegistry.cs Registers external-memory-handle contract access.
src/​coreclr/​vm/​gcenv.ee.cpp Integrates external handles into GC root scanning.
src/​coreclr/​vm/​externalmemoryhandle.h Defines external-handle structures.
src/​coreclr/​vm/​externalmemoryhandle.cpp Implements external-handle scanning.
src/​coreclr/​vm/​datadescriptor/​datadescriptor.inc Adds runtime data descriptors.
src/​coreclr/​vm/​CMakeLists.txt Includes the new native source in builds.
src/​coreclr/​vm/​appdomain.hpp Declares AppDomain handle storage and APIs.
src/​coreclr/​vm/​appdomain.cpp Manages, scans, and dumps external handles.
src/​coreclr/​inc/​CrstTypes.def Defines the external-handle lock.
src/​coreclr/​inc/​crsttypes_generated.h Provides generated lock metadata.
src/​coreclr/​debug/​inc/​dbgipceventtypes.h Defines debugger disposal event metadata.
src/​coreclr/​debug/​inc/​dbgipcevents.h Extends debugger IPC payloads.
src/​coreclr/​debug/​ee/​funceval.cpp Protects byref-like func-eval results. Critical: unboxed return data can still be treated as an object for strong-handle creation.
src/​coreclr/​debug/​ee/​debugger.h Declares debugger external-handle support.
src/​coreclr/​debug/​ee/​debugger.cpp Creates and disposes debugger handles.
src/​coreclr/​debug/​di/​rsthread.cpp Transfers and cleans up func-eval handles. Moderate: failed disposal events can leak the handle and buffer.
src/​coreclr/​debug/​di/​rspriv.h Stores debugger-side handle state.
src/​coreclr/​debug/​di/​process.cpp Processes handle disposal events.
src/​coreclr/​debug/​di/​divalue.cpp Associates handles with debugger values. Moderate: clears the token without safely handling IPC-send failure.
src/​coreclr/​debug/​daccess/​dacdbiimpl.h Declares DAC external-root state.
src/​coreclr/​debug/​daccess/​dacdbiimpl.cpp Enumerates external roots through DAC.
docs/​design/​datacontracts/​RuntimeTypeSystem.md Documents runtime type metadata.
docs/​design/​datacontracts/​ExternalMemoryHandles.md Documents the external-memory-handle contract.
docs/​design/​datacontracts/​data-descriptor-meanings.json Documents new descriptor meanings.

Comment thread src/coreclr/debug/ee/funceval.cpp Outdated
@jkotas
jkotas requested a review from a team September 15, 2026 19:24
Comment thread src/coreclr/inc/CrstTypes.def Outdated
@jkotas

jkotas commented Sep 15, 2026

Copy link
Copy Markdown
Member

Have you been able to observe debugger crashes or misbehaviors that are fixed by this change?

@janvorli

Copy link
Copy Markdown
Member

@jkoritzinsky I haven't heard about external memory handles before. What do we plan to use them for?

Comment thread docs/design/datacontracts/data-descriptor-meanings.json Outdated
@jkoritzinsky

Copy link
Copy Markdown
Member Author

Have you been able to observe debugger crashes or misbehaviors that are fixed by this change?

I have not observed any debugger crashes, but I did find in the func-eval code that we create a boxed byreflike on the heap to represent the result of the funceval:

if ((pDE->m_evalType != DB_IPCE_FET_NEW_OBJECT) && !RetValueType.IsNull())
{
ValidateFuncEvalReturnType(pDE->m_evalType, RetValueType.GetMethodTable());
RetValueType.GetMethodTable()->EnsureInstanceActive();
retObject = AllocateObject(RetValueType.GetMethodTable());

This is how we currently don't have a bunch of GC holes with funceval returns, but it's definitely not supported.

We block allocating byreflike values on the GC heap in all of the boxing paths:

// we shouldn't allow boxing of types that contains stack pointers
// csc and vbc already disallow it.
if (VMClsHnd.AsMethodTable()->IsByRefLike())
COMPlusThrow(kInvalidProgramException,W("NotSupported_ByRefLike"));

// Don't allow ref structs
if (!allowByRefLike && pMT->IsByRefLike())
{
COMPlusThrow(kNotSupportedException, W("NotSupported_ByRefLike"));
}

if (IsByRefLike())
{
// We should never box a type that contains stack pointers.
COMPlusThrow(kInvalidOperationException, W("InvalidOperation_TypeCannotBeBoxed"));
}

However, we don't block it in AllocateObject itself:

// AllocateObject will throw OutOfMemoryException so don't need to check
// for NULL return value from it.
OBJECTREF AllocateObject(MethodTable *pMT
, GC_ALLOC_FLAGS flags
#ifdef FEATURE_COMINTEROP
, bool fHandleCom
#endif
)
{
CONTRACTL {
THROWS;
GC_TRIGGERS;
MODE_COOPERATIVE; // returns an objref without pinning it => cooperative
PRECONDITION(CheckPointer(pMT));
PRECONDITION(pMT->CheckInstanceActivated());
} CONTRACTL_END;
// use unchecked oref here to avoid triggering assert in Validate that the AD is
// not set becuase it isn't until near the end of the fcn at which point we can allow
// the check.
_UNCHECKED_OBJECTREF oref;
SetTypeHandleOnThreadForAlloc(TypeHandle(pMT));
#ifdef FEATURE_COMINTEROP
#ifdef FEATURE_COMINTEROP_UNMANAGED_ACTIVATION
if (fHandleCom && pMT->IsComObjectType())
{
if (!g_pConfig->IsBuiltInCOMSupported())
{
COMPlusThrow(kNotSupportedException, W("NotSupported_COM"));
}
// Create a instance of __ComObject here is not allowed as we don't know what COM object to create
if (pMT == g_pBaseCOMObject)
COMPlusThrow(kInvalidComObjectException, IDS_EE_NO_BACKING_CLASS_FACTORY);
OBJECTREF obj = AllocateComObject_ForManaged(pMT);
oref = OBJECTREF_TO_UNCHECKED_OBJECTREF(obj);
}
#endif // FEATURE_COMINTEROP_UNMANAGED_ACTIVATION
#else // FEATURE_COMINTEROP
if (pMT->IsComObjectType())
{
COMPlusThrow(kPlatformNotSupportedException, IDS_EE_ERROR_COM);
}
#endif // FEATURE_COMINTEROP
else
{
if (pMT->ContainsGCPointers())
flags |= GC_ALLOC_CONTAINS_REF;
if (pMT->HasFinalizer())
flags |= GC_ALLOC_FINALIZE;
DWORD totalSize = pMT->GetBaseSize();
if (totalSize >= LARGE_OBJECT_SIZE && totalSize >= GCHeapUtilities::GetGCHeap()->GetLOHThreshold())
flags |= GC_ALLOC_LARGE_OBJECT_HEAP;
#ifdef FEATURE_64BIT_ALIGNMENT
if (pMT->RequiresAlign8())
{
// The last argument to the allocation, indicates whether the alignment should be "biased". This
// means that the object is allocated so that its header lies exactly between two 8-byte
// boundaries. This is required in cases where we need to mis-align the header in order to align
// the actual payload. Currently this is false for classes (where we apply padding to ensure the
// first field is aligned relative to the header) and true for boxed value types (where we can't
// do the same padding without introducing more complexity in type layout and unboxing stubs).
_ASSERTE(sizeof(Object) == 4);
flags |= GC_ALLOC_ALIGN8;
if (pMT->IsValueType())
flags |= GC_ALLOC_ALIGN8_BIAS;
}
#endif // FEATURE_64BIT_ALIGNMENT
Object* orObject = (Object*)Alloc(totalSize, flags);
if (flags & GC_ALLOC_USER_OLD_HEAP)
{
orObject->SetMethodTableForUOHObject(pMT);
}
else
{
orObject->SetMethodTable(pMT);
}
PublishObjectAndNotify(orObject, flags);
oref = OBJECTREF_TO_UNCHECKED_OBJECTREF(orObject);
}
return UNCHECKED_OBJECTREF_TO_OBJECTREF(oref);
}

This PR gets us off this unsupported path.

I haven't heard about external memory handles before. What do we plan to use them for?

I plan to use them basically for supporting func-eval (this PR allocates a handle for the return of a funceval). In a future PR, I want to use them to replace the func-eval use cases for ProtectValueClassFrame as well (byreflike args that can't be boxed), enabling me to close #133730 in favor of this PR. The other usages of ProtectValueClassFrame today can all accept boxing (RuntimeMethodInfo.Invoke) so those cases will just move to the boxing path.

CCR flagged that the strong-handle path treated any non-null RetValueType
as a boxed object, including byref-like results whose slot holds value
data copied into the external memory handle rather than an OBJECTREF.
Exclude byref-like RetValueType from that condition; the external
memory handle remains the sole GC root for byref-like func-eval results.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 15, 2026 21:17
The new lock has no dependencies with other Crst types, so it can
participate in ranking instead of being explicitly unordered.
Regenerated crsttypes_generated.h via CrstTypeTool.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

End-to-end CoreCLR coverage is still requested for real external-handle GC scanning and retention.

Review tier: Lite
Findings: None

Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 15, 2026 21:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Cleanup and partial-dump handling issues remain, end-to-end func-eval lifetime coverage is missing, and public API approval is unresolved.

Review tier: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Add an end-to-end debugger lifetime regression test

src/​coreclr/​debug/​ee/​funceval.cpp:2821

These changes allocate/register a real GC root and transfer ownership across the debugger IPC boundary, but the added tests only use mocked cDAC targets; none exercises actual FuncEvalComplete/GC/ICorDebugValue lifetime or cleanup after GetResult. Add an end-to-end debugger regression that forces a collection while the returned byref-like value is held and verifies disposal after release; this is the behavior the native changes are intended to protect.

[!NOTE] This review comment was generated by GitHub Copilot.

Comment thread src/coreclr/vm/externalmemoryhandle.cpp Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3900c560-18f8-4900-9840-ae7022248647
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3900c560-18f8-4900-9840-ae7022248647
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3900c560-18f8-4900-9840-ae7022248647
Copilot AI review requested due to automatic review settings September 18, 2026 20:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new IPC event is missing transport-size handling, and the GC-map refactor regresses best-effort handling of unreadable field metadata.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)

Comment thread src/coreclr/debug/inc/dbgipceventtypes.h

@max-charlamb max-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my POV regarding the cDAC

…PC event sizing

- Add missing DB_IPCE_DISPOSE_EXTERNAL_MEMORY_OWNER case to GetEventSize
  so the payload is not truncated and the owner handle is not leaked.
- Narrow ByRefPointerOffsetsReporter's best-effort catch to
  InvalidOperationException only, since NotImplementedException cannot
  actually be thrown by any IRuntimeTypeSystem member it calls.
- Make best-effort behavior the only mode for Find(), removing the
  bestEffort parameter and updating the CallingConvention_1 call site.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 18:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The external-root DAC mapping and additional correctness, API, documentation, and regression-test issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 2 Medium severity

Open (4)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Obtain API approval for new public cDAC abstractions

src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Abstractions/​Contracts/​IExternalMemoryHandles.cs:14

This adds ExternalMemoryHandleRootData and IExternalMemoryHandles as new public API in the packable cDAC Abstractions assembly, but the supplied PR description has no linked issue carrying the api-approved label. Link the approved proposal and match its final shape, or keep the new public surface internal until API approval exists.

Comment thread src/coreclr/vm/externalmemoryhandle.h Outdated
Export the external memory handle list through the classic DAC globals and materialize it before traversal. Preserve best-effort cDAC enumeration when target field reads throw VirtualReadException.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 23, 2026 18:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical, moderate, and API-approval findings remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Obtain API approval for new public external-memory contracts

src/​native/​managed/​cdac/​Microsoft.Diagnostics.DataContractReader.Abstractions/​Contracts/​IExternalMemoryHandles.cs:20

This adds a new public IExternalMemoryHandles contract (and the public ExternalMemoryHandleRootData type) under Microsoft.Diagnostics.DataContractReader.Abstractions, but the PR metadata contains no linked issue with an api-approved label. The cDAC exception covers implementations under Contracts/**, not this new public abstraction surface; link the approved proposal or keep the new API internal until approval.

Comment thread src/coreclr/debug/ee/debugger.cpp
@jkoritzinsky

Copy link
Copy Markdown
Member Author

/ba-g SOSTests failure is #134603

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

8 participants